feat(core): Extend TurboModule instrumentation to legacy NativeModules#6504
Draft
alwx wants to merge 11 commits into
Draft
feat(core): Extend TurboModule instrumentation to legacy NativeModules#6504alwx wants to merge 11 commits into
alwx wants to merge 11 commits into
Conversation
Adds per-span attribution to `turboModuleContextIntegration`: on root
span end, writes `turbo_module.<name>.<method>.{call_count,duration_ms,
error_count}` attributes plus summary keys. Async calls above
`slowCallThresholdMs` (default 500ms) also emit a `native.turbo_module`
breadcrumb. New knobs: `enableSpanAttribution`, `slowCallThresholdMs`,
`maxTopModulesPerSpan`.
Closes #6165.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Nested `name→method` counter map so identifiers with any character can't collide (also removes a stray NUL byte in the compound key). - Fire per-record observers even when `enableAggregateStats: false`, and apply `ignoreTurboModules` whenever any consumer of the record path is active. Fixes span attribution + slow-call breadcrumb going silent when aggregate stats are opted out. - Snapshot the set of open windows at call start via a new `notifyTurboModuleCallStart` hook, so async calls that outlive their originating span still credit that span. Late-settling records re-emit `setAttributes` on the (now closed) span. - CHANGELOG entry references PR #6478 instead of the issue. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`startObserver` used to skip writing to `pendingCallWindows` when the snapshot was empty. When the call settled, `recordObserver` couldn't find the recordId and fell back to the currently-open windows — crediting spans that opened *after* the call started. Common case: an async TurboModule call kicked off during app init, before any nav / user span has opened. Always record a snapshot (even an empty one), and reserve the currently-open fallback for records without a recordId (direct `recordTurboModuleCall` callers in tests). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Clear per-method attribute keys from a previous emit that no longer fit in the top-N. `setAttributes` merges, so without this a late-settling async call that re-ranks the top-N would leave dropped rows on the span. - Cap `pendingCallWindows` at 1024 so abandoned (never-settling) async promises can't pin `WindowState` indefinitely. Evicted entries are silently dropped on late settle rather than mis-attributed. - Correct the misleading comment about the WeakMap — it enables O(1) `spanEnd` lookup; it does not protect against leaks (the parallel `openWindowList` holds strong refs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Register the record observer whenever either `enableSpanAttribution` or `slowCallThresholdMs > 0` is active, and gate span-attribution and breadcrumb logic separately inside. Previously the breadcrumb code lived inside the `enableSpanAttribution` block, so disabling span attribution silently turned off breadcrumbs — contradicting the documented independent `slowCallThresholdMs` knob. `setIgnoredTurboModules` also now applies in breadcrumbs-only mode. - Sync calls skip `pendingCallWindows`. Sync start + settle happen in the same turn, so they can credit `openWindowList` directly. Skipping them also prevents sync bursts from evicting genuine async entries under the MAX_PENDING_CALL_WINDOWS cap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`wrapTurboModule` always calls `notifyTurboModuleCallStart` with kind `'sync'` and only relabels to `'async'` once the return value is known to be thenable. Gating the start observer on `kind === 'async'` therefore silently dropped every async attribution in production. Revert to snapshotting on every start — sync entries settle in the same synchronous turn and are removed by their paired record, so they don't accumulate under normal traffic. Also add the missing blank line between the Features and Fixes sections in the CHANGELOG. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Late-settling async records after `spanEnd` write via
`span.setAttributes` to a frozen span, which the Sentry SDK silently
drops. Also buffer the attribute payload by span_id and merge into
`event.contexts.trace.data` in `processEvent` so the update lands on
the transaction that ships. `MAX_PENDING_SPAN_ATTRIBUTES` caps the
buffer against sampled-out transactions never coming back to claim
their entry.
- Escape `.` in module/method names when composing attribute keys.
`turbo_module.${name}.${method}.*` collides otherwise —
`(name="a.b", method="c")` and `(name="a", method="b.c")` both produce
`turbo_module.a.b.c.*` and overwrite each other. Same fix applied to
the aggregate flush serialisation.
- Fix the pendingCallWindows cap-eviction test: previously the fillers
never settled, so `attachWindowToSpan` returned early on empty
counters and the assertion loop never ran. Settle a filler so the
eviction outcome is actually verified.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
Contributor
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Extend TurboModule instrumentation to legacy NativeModules ([#6504](https://github.com/getsentry/sentry-react-native/pull/6504))If none of the above apply, you can opt out of this check by adding |
Prune verbose block comments that restated the code or referenced issues. Kept one-line WHYs where the reasoning is load-bearing (frozen-span setAttributes, dot-collision escape, merge semantics). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wraps `NativeModules.*` on the Old Architecture so bridge calls flow through the same aggregator, span-attribution, and slow-call breadcrumb surface as TurboModules on the New Architecture. - Adds `arch: 'new' | 'legacy'` to TurboModuleRecord / TurboModuleAggregate / TurboModuleCallStart, threaded through `wrapTurboModule`, and folded into the aggregate key so new-arch and legacy calls stay separate on flush. - New `wrapAllNativeModules` enumerates `NativeModules` on the Old Arch and wraps each registered module (skipping RNSentry, which the integration wraps explicitly with a curated recursion-safe skip list). - `turboModuleContextIntegration` auto-invokes it on the Old Arch. Opt out with `enableLegacyNativeModules: false`, or scope with `legacyModulesSkip` / `legacyModulesSkipMethods`. - Aggregate flush emits `turbo_modules.<name>.<method>.<kind>.arch`; attributed spans get a top-level `turbo_module.arch` derived from `isTurboModuleEnabled()` at integration construction (an app doesn't switch archs at runtime). Closes #6166. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e8e56a6. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 Type of change
📜 Description
Extends the TurboModule aggregate + span attribution + slow-call breadcrumb surface (from #6377 and #6478) to Old Architecture apps by wrapping
NativeModules.*at setup. Consumer side stays identical — only thearchfield distinguishes new-arch ('new') vs bridge ('legacy') sources.Approach
This ships the JS Proxy path. The native
BridgeNativeModulePerfLoggerpath (option 1) is symmetric but requires new C++/JNI code and a version gate — left for a follow-up. The JS approach works on every RN version we support and has the same shape as the existing TurboModule wrap; hot-path cost is bounded (one property access + function call per bridge invocation).💡 Motivation and Context
TurboModulePerfLoggeronly fires for TurboModules, so apps on the Old Architecture were dark to the entire aggregator / span-attribution / breadcrumb surface. This PR closes that gap so the long tail of un-migrated apps gets the same signals.Closes #6166.
💚 How did you test it?
There are a few tests added!
📝 Checklist
sendDefaultPIIis enabled.